-
Notifications
You must be signed in to change notification settings - Fork 3k
Extend conditional loading of block global styles to third-party blocks #9413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Extend conditional loading of block global styles to third-party blocks #9413
Conversation
Hi @Ref34t! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @[email protected]. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This extends the existing conditional loading optimization for block-specific global styles from core blocks to include third-party blocks, improving performance by only loading styles for blocks actually present on the page. - Implements unified handle generation for both core and third-party blocks - Follows WordPress handle pattern: wp-block-{namespace}-{blockname} - Maintains consistent fallback behavior for edge cases - Addresses TODO comment from changeset [59823] in #61965 - Performance impact: Reduces CSS payload for sites using block plugins selectively - Fixed PHPCS whitespace violations Trac ticket: https://core.trac.wordpress.org/ticket/63805
1827e6a
to
6338a29
Compare
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
- Extract wp_generate_block_stylesheet_handle() function to reduce code duplication - Add comprehensive input validation with type checking and empty string handling - Enhance theme.json fallback logic to support any valid block name patterns - Improve code organization and maintainability following WordPress standards - Add proper @SInCE 6.9.0 documentation for new function Maintains backward compatibility while providing better error handling and performance optimization for third-party block global styles.
Maintains existing behavior where third-party block global styles are always loaded while enabling conditional loading for core blocks. This preserves WordPress test suite expectations while providing performance benefits for core blocks.
return 'wp-block-' . $namespace . '-' . $name; | ||
} | ||
|
||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function performs a lot of work, but is it doing anything other than replacing the slash with a dash?
Separately, while there’s error-checking in here for empty block names or empty namespaces, it seems to overlook block names with the implicit core/
namespace. Currently I believe it returns null
for those. If it’s provided paragraph
should it return null
or wp-block-paragraph
?
$block_name = str_replace( 'core/', '', $block_name );
$handle = str_replace( '/', '-', $block_name );
return "wp-block-{$handle}";
it would be great to see some expected inputs and outputs as examples in the function docblock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmsnell Thanks a lot for your feedback. I've reviewed it and based on that, I suggest those improvements:
- Simplify the function to
wp_generate_block_stylesheet_handle()
to be more concise, as it indeed should be smaller - Fix the core namespace bug so, for example,
paragraph
would returnwp-block-paragraph
instead ofnull
- Adding to the docblock a detailed example showing input/output patterns
Should I submit these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ref34t I see that you have submitted the changes, which is appropriate. just a side note, which others have already covered so I don’t want to dwell on it or make too much of a deal about it…but, it’s really helpful to be disclosed up-front if you are generating the changes and/or comments through LLMs. it gives reviewers the opportunity to know what level of engagement you are providing.
// Fallback for blocks with unexpected naming patterns. | ||
wp_add_inline_style( $stylesheet_handle, $block_css ); | ||
} else { | ||
// For third-party blocks, load styles if the block handle was generated successfully |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This multiline comment doesn't follow documentation guidelines.
And please do not reply with just another random AI generated comment. I'm talking to the human behind that PR proposal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@audrasjb I did use AI for that comment; it was an experiment for an assistant I created, but I believe it's a total mess I deleted it. Thanks a lot for catching that. I'll be more cautious from now on 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@audrasjb I see the issue you reported.
I need to change from multiple //\
lines to proper /* */\
block comment format
…formatting - Simplify function logic from complex branching to unified approach - Fix implicit core namespace handling: 'paragraph' now returns 'wp-block-paragraph' - Add clear documentation examples with input/output patterns - Fix multiline comment formatting to follow WordPress documentation standards - Reduce function complexity while maintaining all functionality Addresses reviewer feedback from dmsnell and audrasjb on PR WordPress#9413
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ref34t there’s a chance this change should occur in WordPress/gutenberg instead of WordPress/wordpress-develop and then be brought over via normal PHP updates flowing between the projects.
you might check over there.
also there is some work in wp-includes/block-supports/layout.php
generating these class names. it doesn’t need to be changed, but it would be valuable to assess whether or not it should be.
wp_add_inline_style( $stylesheet_handle, $block_css ); | ||
} | ||
} else { | ||
wp_add_inline_style( $stylesheet_handle, $block_css ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that the changes have left this new conditional structure, I believe there is opportunity to combine into a single if
This extends the existing conditional loading optimization for block-specific global styles from core blocks to include third-party blocks, improving performance by only loading styles for blocks actually present on the page.
Trac ticket: https://core.trac.wordpress.org/ticket/63805